Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Recursive Item Mapping for Nested Lists in Compose #8187

Merged
merged 21 commits into from
Feb 12, 2025
Merged

Recursive Item Mapping for Nested Lists in Compose #8187

merged 21 commits into from
Feb 12, 2025
+57 −17

Conversation

KumoLiu
Copy link
Contributor

@KumoLiu KumoLiu commented Nov 1, 2024

Fixes #8186.

Description

A few sentences describing the changes proposed in this pull request.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@ziw-liu
Copy link

ziw-liu commented Jan 3, 2025

Hi @KumoLiu and thanks for the fix! Is this branch ready for testing?

@KumoLiu
Copy link
Contributor Author

KumoLiu commented Jan 6, 2025

Hi @KumoLiu and thanks for the fix! Is this branch ready for testing?

Hi @ziw-liu, the current fix solves the problem you raised, but it leads to something else, such as not having a handle for those who don't want a recursive map. I still need to revise this PR further. Thanks.

@ericspod
Copy link
Member

@atbenmurray please review this as well as we just discussed.

atbenmurray
atbenmurray requested changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@atbenmurray atbenmurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at it, map_items=True has always acted as map_items=1, so at least we aren't changing the behaviour of using bool and now the int indicates the maximum amount of nesting that can be expected. This would also be the case if we use a multi sample transform after a multi sample transform. Its a bit opaque semantically that we do this and might benefit from additional commenting.

monai/transforms/transform.py Outdated Show resolved Hide resolved
@KumoLiu
Copy link
Contributor Author

KumoLiu commented Jan 24, 2025

Hi @atbenmurray, thanks for the suggestion, I enhance the docstring in the latest commit. Please help take a look whether it looks clearer to you.

atbenmurray
atbenmurray requested changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@atbenmurray atbenmurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor tweak required due to a typo in my earlier suggestion

monai/transforms/transform.py Outdated Show resolved Hide resolved
atbenmurray
atbenmurray approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@atbenmurray atbenmurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with these comments!

@ericspod
Copy link
Member

Hi @KumoLiu we have a minor conflict here but otherwise I think we're good?

@KumoLiu
Copy link
Contributor Author

KumoLiu commented Feb 12, 2025

/build

@KumoLiu
Copy link
Contributor Author

KumoLiu commented Feb 12, 2025

/build

@KumoLiu
fix format
d4bc172 
Signed-off-by: YunLiu <[email protected]>
@KumoLiu
Copy link
Contributor Author

KumoLiu commented Feb 12, 2025

/build

@KumoLiu KumoLiu enabled auto-merge (squash) February 12, 2025 16:43
@KumoLiu KumoLiu merged commit e538f7f into Project-MONAI:dev Feb 12, 2025
28 checks passed
thibaultdvx pushed a commit to thibaultdvx/MONAI that referenced this pull request Feb 13, 2025
@KumoLiu @atbenmurray
@pre-commit-ci @thibaultdvx
Recursive Item Mapping for Nested Lists in Compose (Project-MONAI#8187)
a33a212 
Fixes Project-MONAI#8186.

### Description

A few sentences describing the changes proposed in this pull request.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [ ] New tests added to cover the changes.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

---------

Signed-off-by: YunLiu <[email protected]>
Co-authored-by: Ben Murray <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atbenmurray atbenmurray atbenmurray approved these changes

@Nic-Ma Nic-Ma Awaiting requested review from Nic-Ma

@ericspod ericspod Awaiting requested review from ericspod

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compose cannot work with both Decollated and MultiSampleTrait transforms
4 participants
@KumoLiu @ziw-liu @ericspod @atbenmurray